Feature/aris/presence by ariskotsomitopoulos · Pull Request #4090 · element-hq/element-android

ariskotsomitopoulos · 2021-09-27T14:58:39Z

Presence Management

User Presence: ["online", "offline", "unavailable"]

Presence in DM room list
Presence in DM chat
Presence in all Rooms member details

Issue #70

DoM1niC · 2021-09-27T15:07:14Z

Presence in RoomDetails and in Room would be nice to have :) I like this PR 💯

like Web

gradle.properties

github-actions · 2021-09-27T15:25:47Z

Unit Test Results

  44 files ±0   44 suites ±0 45s ⏱️ -4s
  87 tests ±0   87 ✔️ ±0 0 💤 ±0 0 ❌ ±0
228 runs ±0 228 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit ffa5e1a. ± Comparison against base commit 7338982.

♻️ This comment has been updated with latest results.

ganfra

Some remarks, otherwise, good job (not tried)

...-android/src/main/java/org/matrix/android/sdk/internal/session/presence/di/PresenceModule.kt

...rc/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/ReadReceiptHandler.kt

...trix/android/sdk/internal/session/sync/handler/SyncResponsePostTreatmentAggregatorHandler.kt

...dk-android/src/main/java/org/matrix/android/sdk/internal/database/model/RoomSummaryEntity.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/RoomSummary.kt

vector/src/main/java/im/vector/app/features/home/room/list/RoomSummaryItem.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/presence/PresenceService.kt

...android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt

bmarty · 2021-09-27T16:00:57Z

.../src/main/java/org/matrix/android/sdk/internal/database/model/presence/UserPresenceEntity.kt

+                                       var lastActiveAgo: Long? = null,
+                                       var statusMessage: String? = null,
+                                       var isCurrentlyActive: Boolean? = null,
+                                       var avatarUrl: String? = null


why not storing also the display name?

...oid/src/main/java/org/matrix/android/sdk/internal/database/query/RoomSummaryEntityQueries.kt

...in/java/org/matrix/android/sdk/internal/database/query/presence/UserPresenceEntityQueries.kt

...ix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/presence/PresenceAPI.kt

...-android/src/main/java/org/matrix/android/sdk/internal/session/presence/di/PresenceModule.kt

...main/java/org/matrix/android/sdk/internal/session/presence/service/DefaultPresenceService.kt

...c/main/java/org/matrix/android/sdk/internal/session/presence/service/task/SetPresenceTask.kt

...id/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/PresenceSyncHandler.kt

vector/src/main/res/layout/item_room.xml

bmarty

Good work, I know the PR is still Draft, I've just made first remarks (static review).

...roid/src/main/java/org/matrix/android/sdk/internal/session/presence/model/PresenceContent.kt

bmarty · 2021-09-28T08:31:29Z

...main/java/org/matrix/android/sdk/internal/session/presence/service/DefaultPresenceService.kt

+        setPresenceTask.execute(SetPresenceTask.Params(userId, presence, message))
+    }
+
+    override suspend fun fetchPresence(userId: String) = getPresenceTask.execute(GetPresenceTask.Params(userId))


As said by @ganfra you should add a method to get LiveData of a user presence (or if you want to use Flow, it would be better!), to make it live in the UI.

...k-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomSummaryMapper.kt

ganfra

Some small remaining remarks

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/presence/PresenceService.kt

...roid/src/main/java/org/matrix/android/sdk/internal/session/presence/model/PresenceContent.kt

bmarty · 2021-10-01T14:43:40Z

...roid/src/main/java/org/matrix/android/sdk/internal/database/model/RoomMemberSummaryEntity.kt

+    var userPresence: UserPresenceEntity? = null
+        set(value) {
+            if (value != field) field = value
+        }


I'm not sure it make sense to test != for Realm object, @BillCarsonFr WDYT?

Yeah, I believe it does not make sense, but there are other implementations like this in RoomSummaryEntity:

var latestPreviewableEvent: TimelineEventEntity? = null set(value) { if (value != field) field = value }

var userDrafts: UserDraftsEntity? = null set(value) { if (value != field) field = value }

So if we decide to remove it let's remove it from the other places too, if you agree

...oid/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomMemberSummaryMapper.kt

bmarty · 2021-10-01T14:49:41Z

...roid/src/main/java/org/matrix/android/sdk/internal/database/query/RoomMemberEntityQueries.kt

+            .equalTo(RoomMemberSummaryEntityFields.USER_ID, userId)
+            .findAll()
+            .map {
+                it.userPresence = userPresenceEntity


is it necessary to do this? The link between it.userPresence and userPresenceEntity is always the same, no?

Well, I think you are partially right, we need that only for the first linking, after that it will auto update the changes, so I simply add in the query .isNull(RoomMemberSummaryEntityFields.USER_PRESENCE.$) so now it will only do that for the first linking. Do you think there is a better way for the initial linking with Realm?

...android/src/main/java/org/matrix/android/sdk/internal/session/presence/model/UserPresence.kt

bmarty · 2021-10-01T14:52:55Z

.../main/java/org/matrix/android/sdk/internal/session/room/membership/RoomMemberEventHandler.kt

+                roomId,
+                userId,
+                roomMember,
+                // To resolve an edge cases like, on initial sync RoomMemberEventHandler is called after PresenceSyncHandler,


maybe we can change and call PresenceSyncHandler after RoomMemberEventHandler?

Well, I deep dived into the problem about what is happening. I will also add a comment in the codebase explaining that. Let me explain, when we use insertOrUpdate then we have two results:

Insert : A brand new object is created with a primary key etc and is inserted in the DB

Update: A primary key is already found in that table so the framework replace the existing object with the new one.

The problem is when there is an update! With An example will be more clear. Let's say we have the following record:
[ MyPrimaryKey: 123, catName: "Anna", dogName:"John" , .... etc]

and now lets say server has an update with [ MyPrimaryKey: 123, catName: "Ketty"]

So now if we do insertOrUpdate with an object containing only MyPrimaryKey and catName. All the other records will be replaced by null, so after the insertOrUpdate our record will be updated to:
[ MyPrimaryKey: 123, catName: "Anna", dogName:NULL , .... etc]

So to avoid this there are two solutions:

Manually check if the record exists, and update only the fields we want to update

Pick the old value and pass it to new object creation (Faster if the select is with @indexed fields)

While userId and roomId are @indexed, I think the second one is clearer and more efficient. What do you think?

vector/src/main/java/im/vector/app/core/ui/views/PresenceStateImageView.kt

vector/src/main/res/layout/fragment_room_detail.xml

DoM1niC · 2021-10-04T11:13:08Z

Hey Aris,
can you fix the conflicts plz :) I wanne use it in my fork....

THX for all your Work!!!!

DoM1niC · 2021-10-04T15:31:02Z

Circle of offline presence is only transparent instead of filled color

ariskotsomitopoulos · 2021-10-04T15:40:21Z

Circle of offline presence is only transparent instead of filled color

hmm strange, what android do you have device/version? is the same with light mode too?

DoM1niC · 2021-10-04T16:58:32Z

Circle of offline presence is only transparent instead of filled color

hmm strange, what android do you have device/version? is the same with light mode too?

Android 11 on Android Studio yes light mode is the same issue since lastest Commits

DoM1niC · 2021-10-04T17:32:49Z

DoM1niC · 2021-10-04T18:13:29Z

Some Users are fine and some not lol 😂

ariskotsomitopoulos · 2021-10-04T20:33:06Z

Some Users are fine and some not lol 😂

hmm I think your colors are altered, do you use any custom Theme? Maybe thats why as I can see and other colors are altered

ariskotsomitopoulos · 2021-10-05T20:51:31Z

Circle of offline presence is only transparent instead of filled color

Can you check again? It seems like I had to disable it in servers that do not support presence. So now I guess it will work!

DoM1niC · 2021-10-06T01:04:06Z

Circle of offline presence is only transparent instead of filled color

Can you check again? It seems like I had to disable it in servers that do not support presence. So now I guess it will work!

yeah a good workaround the Presence is now a fully avatar instead of the missing image .. The image appears until a presence status is recieved :)

After the User is switched offline / online the presence image is fine anyway.

- Get Presence Status - Set Presence Status * Integrate presence in room details screen * Integrate presence in room people's view * Update UI to support presence * Fix bug when insertOrUpdate was called on RoomMemberEventHandler and override the correct presence value in RoomMemberSummaryEntity * Improve performance on updateUserPresence in RoomMemberSummaryEntity entity * Remarks & linter fixes * Disable presence when there is no m.presence events. In some servers like matrix.org is disabled atm. * Enhance UI Presence on DM room lists to support dark/light theme * Restore missing lines in gradle.properties to speed up debugging

bmarty

LGTM, I added 2 commits with some cleanup

Florian14 · 2021-10-13T07:05:48Z

vector/src/main/res/layout/view_stub_room_member_profile_header.xml

            app:layout_constraintHorizontal_chainStyle="packed"
            app:layout_constraintStart_toStartOf="parent"
-            app:layout_constraintTop_toTopOf="@+id/memberProfileNameView"/>
+            app:layout_constraintTop_toTopOf="@+id/memberProfileNameView" />


I think we should not have @+id for app fields. It is quite enough to have it in the declaration level of the view, even if it is later in the code, it does not change anythink for the compiler and reduce the view references in the code. WDYT @bmarty ?

I agree this is not mandatory to have + for referenced ids, but what would be the main advantage to remove them across the whole codebase?

This will just reduce the view occurrences when you ctrl + click on a viewId. Probably also optimize the code inspection but not sure this is significant

I see, so this is about dev exp (which is important!). Can you create an issue so we can handle that? Having a lint check would be nice too.

ganfra

LGTM

ariskotsomitopoulos marked this pull request as draft September 27, 2021 14:59